-
Notifications
You must be signed in to change notification settings - Fork 375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tracing: Exclude query string for http.url
tag from net/http
instrumentation
#3045
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, @TonyCTHsu! 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you should address @marcotc 's comment first, and I have one question here, but otherwise this seems acceptable.
span.set_tag(Tracing::Metadata::Ext::HTTP::TAG_URL, request.path) | ||
span.set_tag( | ||
Tracing::Metadata::Ext::HTTP::TAG_URL, | ||
Contrib::Utils::Quantization::HTTP.url(request.path, { query: { exclude: :all } }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine as a default, and you've added tests to make sure there aren't any query strings in other integrations.
Is it worth putting this same quantization line in those integrations as well? For consistency? Or is there a drawback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, our http client integrations do not offer settings for quantization, each of them leverage their public interface to provide data.
I believe we should refactor and centralize the quantization mechanism when we offer the feature.
What does this PR do?
Exclude query string for
http.url
tag fromnet/http
instrumentation